Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format the label in the time scale tooltip #5095

Merged
merged 1 commit into from
Jan 13, 2018

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Dec 28, 2017

@simonbrunel this is an alternative implementation of #4583

Before

before

After

after

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to move the calculation of the label format string into a separate function, then call this function only one time at the end of determineDataLimits or buildTicks and cache it in a private variable:

  // ...
  this._format = timeOpts.tooltipFormat || determineLabelFormat(...);

This variable should then be used in getLabelForIndex

getLabelForIndex: function(index) {
  var me = this;
  var data = me.chart.data;
  var label = data.labels && index < data.labels.length ? data.labels[index] : '';
  return momentify(label, me.options.time).format(me._format);
}

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

@simonbrunel thanks for the review! I've updated this PR

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member

Does that mean that currently (master), the default tooltip label is the timestamp? Are there any issues to link to that PR?

@benmccann benmccann force-pushed the tooltip2 branch 2 times, most recently from 2998d3e to eb49a75 Compare January 7, 2018 21:35
@benmccann
Copy link
Contributor Author

benmccann commented Jan 7, 2018

Thanks. I updated it

The current master doesn't do any formatting by default. I see a timestamp because that's what I pass in. Other users might pass in something else like a moment object, but it still won't be formatted. I did not open an issue for it - only a PR

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 7, 2018
@simonbrunel
Copy link
Member

If your data is a formatted string (e.g. 2018-01-07), it would have been displayed as is, but now it would be changed for another hard coded format (2018-01-07 00:00:00), unless the user specify a tooltipFormat? If so, isn't it a breaking change?

@simonbrunel
Copy link
Member

I didn't mean you should have created an issue for this PR, but rather I'm wondering if this bug hasn't been reported by other users and so we would be able to close them with that PR.

@benmccann
Copy link
Contributor Author

Ok, I added an extra check to just use the string if it's provided as a string.

I'm not aware of any other users reporting this issue. I think it's more common to encounter since the labels.source: 'data' which is a pretty recent feature

@simonbrunel
Copy link
Member

Could be related to old #1466

@simonbrunel
Copy link
Member

Looks good, I would add a few unit tests for this feature.

@benmccann
Copy link
Contributor Author

I've added unit tests

@simonbrunel
Copy link
Member

We should test all cases: 'MMM D, YYYY h:mm:ss.SSS a', 'MMM D, YYYY h:mm:ss a' and 'MMM D, YYYY', but also preserving a string.

if (momentDate.millisecond() !== 0) {
return 'MMM D, YYYY h:mm:ss.SSS a';
}
if (momentDate.second() !== 0 || momentDate.minute() !== 0 || momentDate.hour() !== 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens at 00:00:00 ie, midnight on a day? Or is there no way to detect a time set with that from a moment object that has no time at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chart.js user is specifying timestamps if we're utilizing this function. With timestamps there's no way to differentiate dates vs date plus time of midnight. However, if every single timestamp in the dataset is midnight then it's relatively clear that they're representing dates. Also, I don't know that there's any practical difference between date vs date+midnight.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, if there is at least one time other than midnight, then it will pick 'MMM D, YYYY h:mm:ss a', else if all times are midnight then it will pick 'MMM D, YYYY', which is ok I guess.

@simonbrunel
Copy link
Member

@etimberg Good point and I'm wondering again if it would not be easier to simply use by default the same format as the scale tick label as asked in #1466?

@benmccann I know that you are trying to show more detailed information, but it's one use case and I'm not sure that's the most expected one. If the user wants a more detailed format, he can easily set the expected format in the option, so I'm not sure it worth the extra logic.

@benmccann benmccann force-pushed the tooltip2 branch 2 times, most recently from 6b16ca5 to 52f9331 Compare January 9, 2018 16:57
@benmccann
Copy link
Contributor Author

I've updated the tests

@benmccann benmccann force-pushed the tooltip2 branch 3 times, most recently from e21e78f to 6c88c38 Compare January 11, 2018 17:50
@simonbrunel
Copy link
Member

I'm ok with the code and tests, a bit less with the logic. I would have chosen a simpler path to pick the tooltip format, maybe based on the unit or the tick format (or simply always use the most detailed) but not from the actual data. I don't think there will be big issues with that change but I'm not fan of unstable defaults (e.g. same data displayed in different tz generates different tooltip format).

Not a big deal though, should be fine to merge!?

/cc @etimberg

@etimberg
Copy link
Member

@simonbrunel if you're okay with it, let's merge it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants